Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add standalone version of LPF (#164) #192

Open
wants to merge 2 commits into
base: ros2-master
Choose a base branch
from
Open

Add standalone version of LPF (#164) #192

wants to merge 2 commits into from
+222 −60

Conversation

roncapat
Copy link
Contributor

@roncapat roncapat commented Mar 2, 2024 •
edited
Loading

Closes #164.
Test passes locally.

The old LowPassFilter class is now LowPassFilterRos.
API-breaking since with this patch LowPassFilter is a parent class without ROS parameters.

@christophfroehlich at least for now I wanted to reflect the same difference of Pid and PidROS. But we need to devise a different naming strategy, or this patch will break a lot of perfectly fine code (will require people to swap LowPassFilter with LowPassFilterRos in their projects).

@roncapat roncapat marked this pull request as draft March 2, 2024 20:30
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 67.34694% with 16 lines in your changes are missing coverage. Please review.

Project coverage is 50.38%. Comparing base (759d954) to head (e3fde82).

Additional details and impacted files 
@@               Coverage Diff               @@
##           ros2-master     #192      +/-   ##
===============================================
+ Coverage        50.10%   50.38%   +0.28%     
===============================================
  Files               10       11       +1     
  Lines              497      518      +21     
  Branches           166      170       +4     
===============================================
+ Hits               249      261      +12     
- Misses             217      225       +8     
- Partials            31       32       +1
Flag Coverage Δ
unittests 50.38% <67.34%> (+0.28%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
include/control_filters/low_pass_filter.hpp 80.43% <100.00%> (+5.05%) ⬆️
src/control_filters/low_pass_filter.cpp 100.00% <100.00%> (ø)
include/control_filters/low_pass_filter_ros.hpp 60.00% <60.00%> (ø)

@roncapat roncapat marked this pull request as ready for review March 18, 2024 13:19
@christophfroehlich
Copy link
Contributor

Sorry for the late reply. I see two options:

  • Use a consistent nomenclature and break API with Jazzy (we would have to branch this repo then)
  • Deprecate the old LowPassFilter with the hint to replace it with LowPassFilterRos and call the base filter LowPassFilterBase or similar. This could be then rewritten in a future release to converge back to the consistent nomenclature.

What do you think?

@roncapat
Copy link
Contributor Author

roncapat commented Apr 4, 2024

Sorry for the late reply. I see two options:

  • Use a consistent nomenclature and break API with Jazzy (we would have to branch this repo then)
  • Deprecate the old LowPassFilter with the hint to replace it with LowPassFilterRos and call the base filter LowPassFilterBase or similar. This could be then rewritten in a future release to converge back to the consistent nomenclature.

What do you think?

Yeah I believe we should slowly make nomenclature consistent, I like better solution 2.

BTW, why are there no 'humble', 'iron', 'rolling' branches and so on in this repo?

@roncapat
Copy link
Contributor Author

roncapat commented Apr 4, 2024

I will update the PR as soon as possible implementing what you proposed as option 2.

@christophfroehlich
Copy link
Contributor

BTW, why are there no 'humble', 'iron', 'rolling' branches and so on in this repo?

because the ros2-master branch is released to all distros, there was no need for branching yet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bmagyar bmagyar Awaiting requested review from bmagyar

@destogl destogl Awaiting requested review from destogl

@erickisos erickisos Awaiting requested review from erickisos

@fmauch fmauch Awaiting requested review from fmauch

@olivier-stasse olivier-stasse Awaiting requested review from olivier-stasse

@peterdavidfagan peterdavidfagan Awaiting requested review from peterdavidfagan

@progtologist progtologist Awaiting requested review from progtologist

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

LowPassFilter accepts parameters only from node
3 participants
@roncapat @codecov-commenter @christophfroehlich